🤖 Fix uncommitted changes display using unified git diff #325
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When 'Dirty' checkbox is enabled, uncommitted filesystem changes were not appearing in the code review panel. Root cause: combining git commands with && (e.g., 'git diff base...HEAD && git diff HEAD') produces duplicate FileDiff entries for the same file. The parser was creating separate FileDiff objects for each, meaning: - File tree showed files twice (or one overwrote the other) - Hunks from both diffs contributed to the list - But file filtering only matched hunks from the FIRST entry Solution: Deduplicate FileDiff entries by file path after parsing, merging hunks from duplicate entries into a single canonical entry. This makes the parser more robust and ensures all hunks (committed and uncommitted) appear when filtering by file. Test approach (TDD): 1. Added failing test demonstrating duplicate FileDiff bug 2. Implemented deduplication logic in parseDiff() 3. Verified all tests pass (12 diffParser tests, 628 total tests) _Generated with `cmux`_
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Instead of deduplicating FileDiff entries after parsing, fix the root cause by generating better git commands. **Problem with old approach:** - Used `git diff base...HEAD && git diff HEAD` when includeUncommitted=true - Created TWO separate diffs for same file with different base hashes - Resulted in confusing UX (separate hunks with different contexts) **New approach:** - When includeUncommitted=true for branch diffs: use TWO-DOT diff (`git diff base`) to show unified changes from base to working directory - For --staged: keep && behavior (staged + unstaged shown separately) - For HEAD: no change needed (already shows uncommitted) **Result:** - Single unified diff showing all changes cleanly - No duplicate FileDiff entries - Better UX - reviewers see cohesive changes, not fragmented hunks - Net -5 LoC (simpler codebase) **Testing:** - Updated test to verify single FileDiff with unified changes - Added test for --staged edge case (should still use &&) - All 629 tests pass ✅ - Types check ✅ _Generated with `cmux`_
The test was falling back to hardcoded 'main' which fails on systems where git init creates 'master' by default. Fix: Get the current branch name before checking out the new branch, avoiding reliance on upstream tracking or hardcoded branch names. _Generated with `cmux`_
ammar-agent
changed the title
_Generated with `cmux`_
Added comprehensive documentation to buildGitDiffCommand explaining: - Three-dot vs two-dot diff behavior - Why each case uses different git commands - What includeUncommitted means in each context - Examples for each scenario This makes the logic clearer for future maintainers. _Generated with `cmux`_
- Condensed buildGitDiffCommand logic (removed redundant comments) - Simplified test assertions and comments - Added test for includeUncommitted=false (three-dot diff) case - Net -30 lines, clearer code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
When the "Uncommitted" checkbox is enabled in the Code Review panel, uncommitted filesystem changes don't appear correctly.
Root cause: The implementation used
git diff base...HEAD && git diff HEAD, which produced TWO separate diff outputs for the same file with different base states, creating confusing UX where hunks reference different contexts.Solution
Generate cleaner git commands that produce unified diffs.
Command Generation Logic
Git Diff Types
base...HEAD): Shows only committed changesbase): Shows all changes from base to working directory (unified view)When
includeUncommitted=truefor branch diffs, we now use two-dot to get a single, cohesive diff showing all changes.Benefits
✅ Cleaner UX - Single unified diff instead of fragmented hunks
✅ Fixes root cause - Not just papering over symptoms
✅ Simpler code - Net -5 LoC (removed unnecessary deduplication)
✅ Better mental model - Matches user expectation of "show me all my changes"
Testing (TDD)
--stagededge case (should still produce two diffs: staged + unstaged)Results:
Tests use
buildGitDiffCommanddirectly from implementation to ensure we're testing the real integration.Impact
Generated with
cmux